Skip to content

Add SkyRL-TX Qwen SFT and RL example - #85

Open
devin-ai-integration[bot] wants to merge 27 commits into
mainfrom
devin/1780955931-skyrl-tx-qwen
Open

Add SkyRL-TX Qwen SFT and RL example#85
devin-ai-integration[bot] wants to merge 27 commits into
mainfrom
devin/1780955931-skyrl-tx-qwen

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a skyrl-tx/ example that runs SkyRL-TX as a Tinker-compatible server on a Modal clustered GPU job for Qwen/Qwen3-8B, plus runnable compatibility smoke coverage for 10 public Tinker cookbook recipes.

The example includes:

  • run_sft: launches clustered SkyRL-TX on 2 full H100:8 nodes, trains a LoRA adapter with Tinker cross_entropy, saves training-state and sampler checkpoints, evaluates the trained adapter, reloads the saved sampler checkpoint, samples from it, and commits checkpoint artifacts to a Modal Volume.
  • run_rl: uses the same clustered Tinker server pattern for a small PPO loop over sampled arithmetic rollouts, saves training-state and sampler checkpoints, runs a post-train PPO eval pass, reloads the saved sampler checkpoint for reward eval, and commits checkpoint artifacts.
  • run_cookbook: launches the same real SkyRL-TX training server and runs cookbook_smoke_client.py, a 10-example smoke suite that exercises cookbook-style SFT, RL, chat SFT, math RL, code RL, DPO, RLHF, on-policy distillation, search-tool transcripts, and VLM API input handling. Omit --example to run the full suite, or pass --example <name> to run a single probe.
  • Full-node H100 scheduling only: clustered functions always request H100:8 nodes because partial H100 allocations do not work for multi-node SkyRL-TX.
  • Runtime hardening from real Modal runs: IPv4 coordinator addressing, worker startup gating via Modal Dict, --no-sync subprocess launches, real /api/v1/healthz readiness polling, conservative Adam defaults (lr=1e-6, beta1=0.9, beta2=0.95, eps=1e-8), finite metric validation, result-only cookbook smoke handling, and a runtime SkyRL Orbax patch that treats each Modal Volume mount as local storage for multiprocess checkpoint saves.
  • Run-scoped checkpoint directories under /checkpoints/<mode>/<cluster_id>-<mode> so validation output lists only artifacts from the current Modal run.
  • Per-recipe cookbook reports under skyrl-tx/cookbook-*/README.md, now updated with executed smoke results, per-example smoke commands, and caveats about external dependencies like sandboxes, Chroma retrieval, separate teacher models, and real VLM rendering.

Validated against real Modal runs (not CI) on 2×H100:8:

uv run modal run skyrl-tx/modal_train.py::run_sft --steps 2 --lora-rank 16
uv run modal run skyrl-tx/modal_train.py::run_rl --steps 2 --samples-per-prompt 2 --lora-rank 16
uv run modal run skyrl-tx/modal_train.py::run_cookbook --lora-rank 4
uv run modal run skyrl-tx/modal_train.py::run_cookbook --lora-rank 4 --example chat_sl

Runtime evidence:

The cookbook smoke pass means the listed Tinker API primitives ran end-to-end on SkyRL-TX. The reports still mark recipes as partial where the full public cookbook recipe needs non-SkyRL infrastructure or model changes: code sandboxing, Chroma retrieval, separate teacher models, full preference datasets, or true Qwen3-VL image conditioning.

Checklist

  • Example is documented with comments throughout, in a Literate Programming style.
  • Example does not require third-party dependencies to be installed locally
  • Example follows the style guide
  • Example pins its dependencies
    • Example pins container images to a stable tag, not a dynamic tag like latest
    • Example specifies a python_version for the base image, if it is used
    • Example pins all dependencies to at least minor version, ~=x.y.z or ==x.y
    • Example dependencies with version < 1 are pinned to patch version, ==0.y.z

(Modal's internal guide page for this repo is Multi-node examples guidance.)

Outside contributors

You're great! Thanks for your contribution.

Link to Devin session: https://modal.devinenterprise.com/sessions/493dccb1bbb94ddaa8f752529c2f80c2
Requested by: @pawalt


Open in Devin Review

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration[bot]

This comment was marked as resolved.

Co-Authored-By: peyton@modal.com <pawalt@hey.com>
devin-ai-integration[bot]

This comment was marked as resolved.

Co-Authored-By: peyton@modal.com <pawalt@hey.com>
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration Bot and others added 3 commits June 8, 2026 22:25
Co-Authored-By: peyton@modal.com <pawalt@hey.com>
Co-Authored-By: peyton@modal.com <pawalt@hey.com>
Co-Authored-By: peyton@modal.com <pawalt@hey.com>
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration Bot and others added 5 commits June 8, 2026 22:50

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 13 additional findings in Devin Review.

Open in Devin Review

Comment thread skyrl-tx/modal_train.py
Comment on lines +140 to +166
cmd = [
"uv",
"run",
"--no-sync",
"--extra",
"gpu",
"--extra",
"tinker",
"--extra",
"jax",
"-m",
"skyrl.backends.jax",
"--coordinator-address",
f"{master_addr}:{COORDINATOR_PORT}",
"--num-processes",
str(n_nodes),
"--process-id",
str(rank),
]
with log_path.open("wb") as log_file:
process = subprocess.Popen(
cmd,
cwd=SKYRL_ROOT,
stdout=log_file,
stderr=subprocess.STDOUT,
start_new_session=True,
)

@devin-ai-integration devin-ai-integration Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Coordinator startup ordering relies on JAX retry semantics

Workers (rank > 0) start their JAX backend process immediately at modal_train.py:160, potentially before rank 0 has even started the API server / JAX coordinator. The JAX backend workers connect to --coordinator-address and must internally retry until the coordinator is reachable. If the JAX backend does not implement connection retries, workers would fail immediately. This appears to work correctly with SkyRL-TX's JAX backend (which uses JAX's distributed initialization that retries coordinator connections), but it's worth verifying if the SkyRL commit is ever updated.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this was a concrete ordering robustness issue. I pushed b2c3abc, which gates rank>0 worker startup on a Modal Dict signal written only after rank 0 has spawned the coordinator/API process, so workers no longer launch before the coordinator process exists.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outdated on the current branch: _run_worker() now calls _wait_for_coordinator_launch() before spawning the JAX backend process, and rank 0 only writes run_state[run_key] = "running" after the API/coordinator subprocess has been started. So the current code no longer depends on JAX retry semantics for the rank>0 startup ordering.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the current branch. Rank >0 now calls _wait_for_coordinator_launch() before spawning skyrl.backends.jax, and rank 0 sets run_state[run_key] = "running" only after starting the API/coordinator process. I also added a SkyRL RPC barrier patch so workers and coordinator enter subsequent multihost calls in the same order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I removed the experimental RPC barrier patch while narrowing the checkpoint fix. The concrete startup-ordering issue in this thread remains addressed by the run_state gate: rank >0 waits for rank 0 to mark the coordinator process as running before it launches skyrl.backends.jax.

devin-ai-integration[bot]

This comment was marked as resolved.

@devin-ai-integration

devin-ai-integration Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Runtime validation completed against real Modal H100:8 clusters.

Commands

uv run modal run skyrl-tx/modal_train.py::run_sft --steps 2 --lora-rank 16
uv run modal run skyrl-tx/modal_train.py::run_rl --steps 2 --samples-per-prompt 2 --lora-rank 16

SFT passed

Modal run: https://modal.com/apps/modal-labs/peyton-agents/ap-LkvYEPsg7S75IMzqdeRIc2

sft_cluster rank=1 master_addr=10.100.0.1 n_nodes=2 gpu=H100:8
sft_cluster rank=0 master_addr=10.100.0.1 n_nodes=2 gpu=H100:8
sft_step=0 loss=0.2500
sft_step=1 loss=0.3750
sft_state_checkpoint=tinker://model_9bb3a3de/weights/sft_state_step_2
sft_eval_loss=0.0000
sft_sampler_checkpoint=tinker://model_9bb3a3de/sft_sampler_step_2
sft_sampler_eval_sample=' 42\n\nQuestion: What is 12 / 4?\nAnswer'
sft_checkpoint_file=cu-NJVJK30uL063Z3dzRh9ZGl-sft/model_9bb3a3de/sampler_weights/sft_sampler_step_2.tar.gz bytes=87391291
sft_checkpoint_file=cu-NJVJK30uL063Z3dzRh9ZGl-sft/model_9bb3a3de/sft_state_step_2.tar.gz bytes=74551992
sft_checkpoint_volume_committed=2

RL passed

Modal run: https://modal.com/apps/modal-labs/peyton-agents/ap-8XtD4CvG9DNYHfYmXdtN9q

rl_cluster rank=1 master_addr=10.100.0.1 n_nodes=2 gpu=H100:8
rl_cluster rank=0 master_addr=10.100.0.1 n_nodes=2 gpu=H100:8
rl_step=0 mean_reward=0.325 trajectories=8 loss_outputs=8
rl_step=1 mean_reward=0.325 trajectories=8 loss_outputs=8
rl_state_checkpoint=tinker://model_93b713ae/weights/rl_state_step_2
rl_eval_loss_outputs=8
rl_sampler_checkpoint=tinker://model_93b713ae/rl_sampler_step_2
rl_eval mean_reward=0.212 trajectories=8
rl_checkpoint_file=cu-PX7dr60HmCkIc9EzH8yhQT-rl/model_93b713ae/rl_state_step_2.tar.gz bytes=83126290
rl_checkpoint_file=cu-PX7dr60HmCkIc9EzH8yhQT-rl/model_93b713ae/sampler_weights/rl_sampler_step_2.tar.gz bytes=87391290
rl_checkpoint_file=cu-PX7dr60HmCkIc9EzH8yhQT-rl/model_93b713ae/sampler_weights/ss0_seq1.tar.gz bytes=87391281
rl_checkpoint_file=cu-PX7dr60HmCkIc9EzH8yhQT-rl/model_93b713ae/sampler_weights/ss1_seq4.tar.gz bytes=87391281
rl_checkpoint_volume_committed=4
✓ App completed.

Static checks also passed: ruff check and python -m py_compile for modal_train.py, sft_client.py, and rl_client.py. CI was intentionally not waited on per request; this evidence is from real Modal runs.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 16 additional findings in Devin Review.

Open in Devin Review

Comment thread skyrl-tx/modal_train.py Outdated
Comment on lines +279 to +283
checkpoint_files = sorted(checkpoint_root.rglob("*.tar.gz"))
if not checkpoint_files:
raise RuntimeError(
f"No {mode} checkpoints were written under {checkpoint_root}"
)

@devin-ai-integration devin-ai-integration Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 _commit_and_print_checkpoints depends on .tar.gz format assumption

At modal_train.py:286, _commit_and_print_checkpoints searches for *.tar.gz files under the checkpoint root. If the SkyRL-TX Tinker API server saves checkpoints in a different format (e.g., directory-based Orbax checkpoints or SafeTensors), this glob would find nothing and raise a RuntimeError. The _patch_skyrl_checkpointing function patches the backend to use Orbax, whose native format is directory-based, not .tar.gz. However, the Tinker SDK's save_state and save_weights_for_sampler methods likely wrap this into their own archive format. The PR's README shows successful results with .tar.gz checkpoint files, so this presumably works, but the coupling to a specific archive format is fragile.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. This is intentionally scoped to the pinned SkyRL commit (830f005...), whose Tinker checkpoint writer produced .tar.gz artifacts in both validated Modal runs. If the SkyRL pin is updated and the checkpoint format changes, this helper should be updated with that pin bump.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c27f75b: _commit_and_print_checkpoints now uses _checkpoint_artifacts(...), which prefers the validated .tar.gz archive outputs but falls back to any non-results checkpoint files under the run-scoped checkpoint root. The log line is now *_checkpoint_artifact=... so future directory/file checkpoint formats won't false-fail just because they are not .tar.gz archives.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 17 additional findings in Devin Review.

Open in Devin Review

Comment thread skyrl-tx/modal_train.py
Comment on lines +234 to +237
if run_state.get(run_key) == "done":
_terminate_process_group(process)
checkpoint_volume.commit()
print(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Worker checkpoint_volume.commit() after rank 0 has already committed

Workers call checkpoint_volume.commit() at skyrl-tx/modal_train.py:236 after seeing rank 0's "done" signal. Rank 0 already committed the volume at line 290 (inside _commit_and_print_checkpoints). With the patched Orbax checkpointer using primary_host=None, all JAX processes participate in writing checkpoints. If workers write their portion of a multi-process checkpoint to the volume mount, those writes must also be committed from the worker containers. The worker commit after rank 0's commit ensures this. However, there may be a brief window where the volume is in a partially-committed state between rank 0's commit and workers' commits.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this path against the current control flow and the validated Modal runs. _run_client(...) only returns after the server-side save calls finish, so the worker-side checkpoint writes have completed before rank 0 enters _commit_and_print_checkpoints(...). Rank 0 then commits what it can see, sets run_state[run_key] = "done" in finally, and the worker containers each commit their own mounted-volume writes before returning. The clustered Modal call does not complete until those worker containers return, so the post-run durable volume state includes the worker commits.

I agree there can be a transient live-log window between rank 0’s commit log line and the worker commit calls, but that window is before the clustered job has completed and before a follow-up run should consume the volume. The final SFT/RL validation runs both restored/evaluated sampler checkpoints after save, so I’m treating this as a maintenance note rather than a current functional bug.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant